Generic X.509 certificate helpers and secure file utilities#276
Generic X.509 certificate helpers and secure file utilities#276stenslae wants to merge 4 commits into
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Error: GitHubAPIError
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Error: GitHubAPIError
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Error: GitHubAPIError
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: none
Failed targets: wolfclu-bugs, wolfclu-src
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #276
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 4
3 finding(s) posted as inline comments (see file-level comments below)
Low (1)
RSA private key DER buffer not zeroed when second i2d encoding call fails
Function: wolfCLU_RSA
Category: Sensitive data handling
The PR adds a derSz < 0 guard after the second wolfSSL_i2d_RSAPrivateKey(rsa, &pt) call to detect encoding failure, and a matching if (derSz > 0) guard before wolfCLU_ForceZero to prevent calling ForceZero with a negative size (which would be UB). The combination is logically correct for preventing UB, but it creates a gap: when the second i2d call fails, der has already been allocated to hold the private key DER and may contain partial RSA private key material written by the failing call. Because derSz is now negative, the if (derSz > 0) guard prevents zeroing, so that material is freed without being wiped. The allocation size from the first i2d call is not tracked separately, so it is unavailable for the cleanup. The private key path for public RSA output (wolfSSL_i2d_RSAPublicKey) and the analogous ECC path in _ECCpKeyPEMtoKey (src/pkey/clu_pkey.c:98) share the same pattern. In practice, the second i2d call almost never fails after the first succeeds, so the exploitable window is extremely narrow.
Recommendation: Track the allocation size separately (e.g. word32 allocSz = (word32)derSz; after the first i2d call), then use allocSz in the cleanup: if (allocSz > 0) wolfCLU_ForceZero(der, allocSz);. This ensures zeroing occurs regardless of whether the second i2d call succeeds.
This review was generated automatically by Fenrir. Findings are non-blocking.
| if (ret == WOLFCLU_SUCCESS) { | ||
| file = XFOPEN(fOutNameBuf, "wb"); | ||
| file = wolfCLU_OpenOutFile(fOutNameBuf); | ||
| if (file == XBADFILE) { |
There was a problem hiding this comment.
⚪ [Info] Inconsistent XBADFILE vs NULL check for wolfCLU_OpenOutFile return value in genkey · Code quality / consistency
The PR updated private-key file-open calls from XFOPEN to wolfCLU_OpenKeyFile and checks the result with if (file == NULL). The corresponding public-key file-open calls were updated from XFOPEN to wolfCLU_OpenOutFile, but their error checks were left as if (file == XBADFILE) (lines 1244 and 1493, in wolfCLU_genKey_Dilithium and wolfCLU_genKey_ML_DSA respectively). wolfCLU_OpenOutFile returns FILE* with NULL on failure, and XBADFILE is NULL in all supported wolfSSL build configurations, so the comparison is functionally correct. However, the inconsistency with the private-key check in the same functions (if (file == NULL)) may cause confusion and would break on exotic embedded targets that define XBADFILE to a non-NULL sentinel.
Fix: Change if (file == XBADFILE) to if (file == NULL) at lines 1244 and 1493 to be consistent with the private-key check and to match the actual return type of wolfCLU_OpenOutFile.
| ferror(file)); | ||
| free(buff); | ||
| fclose(file); | ||
| XFREE(buff, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); |
There was a problem hiding this comment.
🟠 [Medium] Missing ForceZero before XFREE of XMSS private-key verification buffer · Missing ForceZero
buff holds a full copy of the XMSS private-key state read back from disk for write verification. The PR changes free(buff) to XFREE(buff, ...) at both the error path (line 1637) and success path (line 1643) without adding wolfCLU_ForceZero, leaving key state in freed heap memory. XMSS statefulness makes this more sensitive than ordinary private-key leakage.
Fix: Add wolfCLU_ForceZero(buff, privSz) before both XFREE(buff, ...) calls in this function.
| /* Open with O_NOFOLLOW instead of fopen() so a symlink swapped in | ||
| * between the lstat() above and this open() is rejected rather | ||
| * than followed. */ | ||
| fd = open(path, access | O_NOFOLLOW); |
There was a problem hiding this comment.
🔵 [Low] TOCTOU symlink window when O_NOFOLLOW is undefined in wolfCLU_CreateSecureFile · File handling vulnerabilities
On platforms where O_NOFOLLOW is not defined (falls back to 0), the special-file branch opens with open(path, access | 0) — no symlink guard — after an lstat() that verified !S_ISLNK. An attacker who can swap the special file for a symlink between lstat() and open() redirects key-file writes to an arbitrary target.
Fix: Treat a failed open() with O_NOFOLLOW=0 as non-fatal only for truly non-secret output; for ownerOnly=1 callers, return NULL if O_NOFOLLOW is unavailable.
2 participants
Added helper functions to do secure file operations. Added simplified certificate parsing and mapping logic into helper functions. Existing modules were refactored to utilize new functions. Added CI tests for all new changes.
This allows for potential reduction in openssl compat layer reliance and new signature algos.